-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add periodic job definitions for ephemeral volumes perf/scalability test cases #13471
add periodic job definitions for ephemeral volumes perf/scalability test cases #13471
Conversation
Welcome @mucahitkurt! |
Hi @mucahitkurt. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/assign @wojtek-t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments, but seems reasonable to me.
@@ -392,3 +392,297 @@ periodics: | |||
annotations: | |||
testgrid-dashboards: sig-scalability-experiments | |||
testgrid-tab-name: gce-private-cluster-correctness | |||
# max volumes per pod test cases for ephemeral volumes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that for now those are a bit experimental tests, I would prefer splitting them into a separate file.
Sth like sig-scalability-experimental-periodic-jobs.yaml
This would make it clear to users that if they won't work, it may be expected, etc.
[if we will do that, we should also move the private-clustester tests there]
@mm4tt - thoughts? concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'm waiting for your final decision about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with Wojtek, +1 to his point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the new storage test jobs and private-cluster job to a new file sig-scalability-experimental-periodic-jobs.yaml
.
- name: ci-kubernetes-storage-scalability-max-emptydir-vol-per-pod | ||
interval: 1h | ||
labels: | ||
#TODO which labels I need? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine - remove a TODO.
- --test-cmd-args=--nodes=1 | ||
- --test-cmd-args=--provider=gce | ||
- --test-cmd-args=--report-dir=/workspace/_artifacts | ||
- --test-cmd-args=--testconfig=testing/experimental/storage/pod-startup/ephemeral-volumes/1_node/config.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it would be a bit more intuitive to add one more override for emptydir (even though it's the default) so that we will know what exactly is happening here (and for consistency with other definitions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, explicit is better. I also will do the same thing for default test case(that's max volume per pod), this will be another PR for perf-tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- --test-cmd-args=--nodes=1 | ||
- --test-cmd-args=--provider=gce | ||
- --test-cmd-args=--report-dir=/workspace/_artifacts | ||
- --test-cmd-args=--testconfig=testing/experimental/storage/pod-startup/ephemeral-volumes/1_node/config.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here about override for emptydir
07649f4
to
cd50373
Compare
be8ef18
to
8ba3d05
Compare
8ba3d05
to
2084f3f
Compare
/retest |
test-infra-bazel seems to be failing - @mucahitkurt - could you please take a look if that's a real issue? |
2084f3f
to
7c41598
Compare
fixed, I forgot the |
This looks fine, but unfortunately there is a conflict now. [Also feel free to ask someone else for lgtm] /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mucahitkurt, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
- --test_args=--ginkgo.flakeAttempts=2 --ginkgo.skip=\[Serial\]|\[Disruptive\]|\[Flaky\]|\[Feature:.+\]|\[DisabledForLargeClusters\] --minStartupPods=8 --node-schedulable-timeout=90m | ||
- --timeout=240m | ||
- --use-logexporter | ||
annotations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just sent out a PR to unify stuff a bit in our configs: #13516
So if you could move annotations right after labels and add an empty line between configs, it would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks!
86bddfc
to
bd83f91
Compare
Rebased, thanks! |
…ility test cases Signed-off-by: Mucahit Kurt <[email protected]>
bd83f91
to
556339c
Compare
/lgtm @mucahitkurt - thanks a lot! |
LGTM label has been added. Git tree hash: 8b025d57d88d8ecd17e32013420d337df1381652
|
@mucahitkurt: Updated the
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@wojtek-t When I check the tests from testgrid, all test cases are passing, but for some test cases, I can't see the podstartup latency artifacts inside the artifacts directory, like this one, I can see for this one. We set the pod startup latency threshold to 5second, when I check the build logs of these test cases I see pod startup latencies over 5seconds, can be the reason why pod startup artifacts are not generated? Is there any dashboard like environment to see the how pod startup latencies are changed across each test run? I remember you mentioned something like that in my PR, but I can't access to it right now(http 500, I think github has some issues). I think I need some support to read/evaluate these test results. |
@mucahitkurt - kubernetes/perf-tests#667 opened |
Periodic job definitions for ephemeral volumes performance/scalability test cases
Meaningful when the test cases PR is merged.
I need support for TODO items.
Fix #13470
cc @wojtek-t @msau42